Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn and do nothing if block is passed to measure command #813

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

tompng
Copy link
Member

@tompng tompng commented Dec 13, 2023

Ruby 3.3 is a good opportunity to add a warning message for measure command with block.
(Although, I think no one reads the warning because (maybe) no one use it.)

I searched the commit that added measure with block but it had no desciption. #183 #184

Measure command with block is

  • Not documented
  • Misleading. Usage is not measure{heavy_logic_to_be_measured}.
  • Hard to use as a command
  • Has alternative way
  • Just making IRB's command system complicated

Usage of measure with block was

# Type this in IRB session.
irb> measure do |context, code, line_no, &block|
irb>   t = Time.now
irb>   result = block.call
irb>   puts "time: #{Time.now - t}"
irb>   result
irb> end
# We already have "TIME" measure, so the actual measure command will be longer and complicated than this example.

Alternative API of measure with block

# Can write this to .irbrc
irb> IRB.conf[:MEASURE_PROC][:FOOBAR] = measure_logic_proc
# Enable it
irb> measure :foobar

# Same as `measure do end`.
irb> IRB.set_measure_callback do |*, &block| measure_logic end

Alternative workaround for measure with block

irb> t=Time.now; measure_target_code; Time.now - t

irb> def m; t = Time.now; [yield, Time.now - t]; end
irb> m { measure_target_code }

Measure with block will register a session-only measure method that you don't need to write to `.irbrc`` for reusing. For this purpose, these workaround is enough and easier than using measure command with block. That's why I think no one use it.

puts "#{added[0]} is added." if added
else
if block_given?
IRB.conf[:MEASURE] = true
Copy link
Member Author

@tompng tompng Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing IRB.conf[:MEASURE] is moved to IRB.set_measure_calback and IRB.unset_measure_calback.
This refactor also makes IRB.set_measure_calback do end to be a complete alternative of measure do end

@@ -235,6 +236,7 @@ def IRB.unset_measure_callback(type = nil)
type_sym = type.upcase.to_sym
IRB.conf[:MEASURE_CALLBACKS].reject!{ |t, | t == type_sym }
end
IRB.conf[:MEASURE] = nil if IRB.conf[:MEASURE_CALLBACKS].empty?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

measure :off, :measure_type had a bug

# lib/irb/cmd/measure.rb:20
when :off
  IRB.conf[:MEASURE] = nil
  IRB.unset_measure_callback(arg)

That disables all measure even if arg is specified.

irb> measure :foo
"FOO" is added
irb> measure :bar
"BAR" is added
irb> measure :off, :foo
# removes :foo and turns all measure off
irb> 42
# no measure
irb> measure
"TIME" added. (and BAR is re-enabled)

@tompng tompng force-pushed the drop_measure_with_block branch from 2f79d19 to 4bda1a3 Compare December 13, 2023 08:32
IRB.unset_measure_callback(arg)
when :list
IRB.conf[:MEASURE_CALLBACKS].each do |type_name, _, arg_val|
puts "- #{type_name}" + (arg_val ? "(#{arg_val.inspect})" : '')
end
when :on
IRB.conf[:MEASURE] = true
added = IRB.set_measure_callback(type, arg)
added = IRB.set_measure_callback(arg)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_measure_callback(:on, arg) had no effect (unless IRB.conf[:MEASURE_PROC][:ON] is set)
This will fix a bug that measure :on was enabling TIME without showing "TIME" is added. message.

This will also allow measure :on, :foobar, an opposite of measure :off, :foobar

Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's such a relief to see none of the recent posts/tweets about measure (example) mentioning the block argument 😄

I think this can help us avoid a condition in #805 too.

@tompng tompng merged commit e79a90a into ruby:master Dec 13, 2023
24 checks passed
@tompng tompng deleted the drop_measure_with_block branch December 13, 2023 12:40
@st0012 st0012 added the enhancement New feature or request label Dec 20, 2023
@nurse nurse mentioned this pull request Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants